I think we might have an edge case that breaks: user manually turns translations off (pref = false) → then turns kill switch ON → middleware reads pref (false) → sets to true. The pref ends up opposite to what the kill switch intended. In TranslationSettingsViewActionType.toggleTranslationsEnabled action let newValue = !current just flips whatever is currently in prefs.

yep you are right. I found this logic a bit strange but was just trying to follow the existing pattern. I think it makes more sense to just pass in the bool value we expect to set it to. What do you think?

Yes. That makes sense.

If modelsClient is nil, the reset is silently skipped, no log, no error. Is it worth at least a logger.log at warning level for the nil case so it's observable if it ever happens in the field?

I'm not familliar with translations, but, I feel like...... should this be hardcoded somewhere central that we can easily keep track of, and we get it from one place so further updates don't get missed out on? Or is this the one place?

I guess I am still not sure where we land on test data.. I have always been in the camp that test data should be hard coded therefore if the code changes the test will catch it. This is not actually needed here so I can just remove it. I copied this from the middleware tests, but generally it is a discussion I would be interested in discussing.

I guess I am still not sure where we land on test data.. I have always been in the camp that test data should be hard coded therefore if the code changes the test will catch it. This is not actually needed here so I can just remove it. I copied this from the middleware tests, but generally it is a discussion I would be interested in discussing.

Yeah, personally, I 100% agree with your stance on test data.

To give some context, in production, supportedLanguages is not a static, hardcoded list. It is resolved at runtime via an async fetch let supported = await modelsFetcher.fetchSupportedTargetLanguages()

ASTranslationModelsFetcher.shared retrieves this data from AS, so the resulting list dynamically reflects the languages currently supported by translation framework at the time of execution.

It feels a bit weird to call this and not end in the assertion. cc: @razvanlitianu I remember we briefly discussed this in one of your PRs. Could you recall what the conclusion was?

Jumping in with less context on reset storage portion:

I may be missing it, but I couldn't find where this was being observed.

Sometimes It feels a bit weird to dispatch this action here and have it heavily tied to the settings side of things, maybe we want it closer to the did reset storage method instead? Might be helpful to understand why we need this dispatch though, but I couldn't find it.

Also realize, we are probably missing a comment here on why we have the 2 dispatched actions. Discussed on one of @razvanlitianu previous PR is due to an issue with how our toolbar configuration is heavily tied to Toolbar Actions and not general Redux actions.

For redux, I was under the impression that we perform the action and anyone that listens would care. So Instead of resetting storage being tied to translation settings, it seems like more related to the model fetcher. Maybe ideally, we have a model fetch middleware that observes the didUpdateSettings action and it calls resetStorage which will dispatch an action when needed.

Sorry for the rambling, just some thoughts. Not a blocker for me if already approved.

cc: @razvanlitianu